-
Notifications
You must be signed in to change notification settings - Fork 41
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
have update_coeffs(L::ADVecProd,) recursively update L.f #232
Conversation
@gaurav-arya, can you take this over and write a few tests in ie change f(y, x) = mul!(y, A, x)
f(x) = A * x to something something like UJacobianWrapper struct Func
u
p
t
end
function update_coefficients!(f::Fun, u, p, t)
# update u, p, t
end
function update_coefficients(f::Fun, u, p, t)
# update u, p, t
end
(f::Fun)(x) = t * p * u |
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## master #232 +/- ##
==========================================
+ Coverage 82.52% 84.76% +2.24%
==========================================
Files 14 14
Lines 944 952 +8
==========================================
+ Hits 779 807 +28
+ Misses 165 145 -20
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report in Codecov by Sentry. |
c6c505d
to
6bb601c
Compare
thanks. can you also add these tests to |
@vpuri3 is the CI working properly? It doesn't seem to have caught the bugs due to ArrayInterface v7, and moreover there's a ref to an object here that doesn't exist:
|
remove zygotejacvec |
file an issue to fix ci after this pr please |
end | ||
|
||
(w::WrapFunc)(u) = sum(w.u) * w.p * w.t * w.func(u) | ||
function (w::WrapFunc)(v, u) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@vpuri3 can you take a look at how this operator is using u
as part of its state, and applying it to a different RHS v
? Is this something that is invalid in your opinion? Because your recent changes don't work with this -- this also relates to the discussion in SciML/SciMLOperators.jl#57.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(This is a separate, SciMLOperators issue, so resolved for our purposes here)
6d73989
to
cc60333
Compare
b6aa232
to
b072b13
Compare
b072b13
to
6cd7300
Compare
@ChrisRackauckas this PR's also ready for review |
… in-place hesvecs, and update tests accordingly
@ChrisRackauckas this is ready for another review. your comment is resolved (see above). There are a few cleanup points for a separate PR (#238), but the behaviour here should now be correct, and ready for ODE.jl. Also: while making the tests more comprehensive I found that |
@@ -110,6 +111,7 @@ function numauto_hesvec!(dy, | |||
g(cache1, x) | |||
@. x -= 2ϵ * v | |||
g(cache2, x) | |||
@. x += ϵ * v |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ChrisRackauckas that this was not done before seems to actually have been a fairly serious numerical error: the vector x where the Jacobian is being evaluated at gets edited by around sqrt(eps) each time the Jacobian is applied, if I am thinking this through correctly
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sorry, not Jacobian; this error was only present for (finite difference computed) Hessians
@ChrisRackauckas ping |
SciML/OrdinaryDiffEq.jl#1917